Skip to content

[Fix] Permanent permissions drift when user_name casing in databricks_permissions access_control blocks differs from the API response#5757

Merged
alexott merged 5 commits into
mainfrom
fix/permissions-drift-user-name-alex
Jun 24, 2026
Merged

[Fix] Permanent permissions drift when user_name casing in databricks_permissions access_control blocks differs from the API response#5757
alexott merged 5 commits into
mainfrom
fix/permissions-drift-user-name-alex

Conversation

@alexott

@alexott alexott commented May 28, 2026

Copy link
Copy Markdown
Contributor

It's just a resubmission of #5391 to workaround the GH actions problems when contributor is outside of the or

Changes

Closes #5183, Fixes #5712

Fixes permanent permissions drift when user_name casing in databricks_permissions access_control blocks differs from the API response.

Problem: The Databricks API normalizes user emails to lowercase (e.g. config has User@Example.com, API returns user@example.com). Because emails are case-insensitive, both refer to the same user, but Terraform treats them as different entries, causing a remove/re-add diff on every plan that never converges.

Fix: Normalize user_name and service_principal_name to lowercase throughout the permissions resource so that casing differences between config and API are ignored. The fix touches three areas:

  1. Hash function: Lowercase user_name and service_principal_name before hashing so config and API values produce the same hash.
  2. API response handling: Lowercase user_name and service_principal_name when reading the API response into state.
  3. Current-user comparison: Use case-insensitive comparison (strings.EqualFold) when checking if the current user is in the config. Without this, the provider silently dropped the current user's permissions from state when their email casing differed, a second source of drift.

Detailed Changes

Hash function case normalization (permissions/resource_permissions.go, lines 191-217)

Before:

if strVal, ok := val.(string); ok {
    if strVal != "" {
        normalized[key] = strVal
    }
}

After:

if strVal, ok := val.(string); ok {
    if strVal != "" {
        if key == "user_name" || key == "service_principal_name" {
            strVal = strings.ToLower(strVal)
        }
        normalized[key] = strVal
    }
}

Why: The access_control block uses schema.TypeSet, which relies on a hash function to determine element identity. If the user's config has User@Example.com and the API returns user@example.com, they produced different hashes, so Terraform saw them as two different set elements: one to remove and one to add. Lowercasing before hashing ensures both values produce the same hash.


API response lowercasing (permissions/permission_definitions.go, lines 247-248)

Before:

entity.AccessControlList = append(entity.AccessControlList, iam.AccessControlRequest{
    GroupName:            accessControl.GroupName,
    UserName:             accessControl.UserName,
    ServicePrincipalName: accessControl.ServicePrincipalName,
    PermissionLevel:      permission.PermissionLevel,
})

After:

entity.AccessControlList = append(entity.AccessControlList, iam.AccessControlRequest{
    GroupName:            accessControl.GroupName,
    UserName:             strings.ToLower(accessControl.UserName),
    ServicePrincipalName: strings.ToLower(accessControl.ServicePrincipalName),
    PermissionLevel:      permission.PermissionLevel,
})

Why: Even with matching hashes, if the stored string value in state differs from the config value, Terraform detects a diff. Lowercasing the API response ensures the state value matches a lowercase config value.


Case-insensitive current-user comparison (permissions/permission_definitions.go, line 231)

Before:

if me == accessControl.UserName || me == accessControl.ServicePrincipalName {

After:

if strings.EqualFold(me, accessControl.UserName) || strings.EqualFold(me, accessControl.ServicePrincipalName) {

Why: == is case-sensitive, so "Admin@Example.com" == "admin@example.com" returns false. strings.EqualFold compares strings case-insensitively, correctly matching the same user regardless of casing.


Case-insensitive lookup in ContainsUserOrServicePrincipal (permissions/entity/permissions_entity.go, line 17)

Before:

if ac.UserName == name || ac.ServicePrincipalName == name {

After:

if strings.EqualFold(ac.UserName, name) || strings.EqualFold(ac.ServicePrincipalName, name) {

Why: Same as above. == is case-sensitive, so "Admin@Example.com" == "admin@example.com" returns false. strings.EqualFold compares strings case-insensitively, correctly matching the same user regardless of casing.


Tests

  • TestPermissionsDrift_UserNameNoDriftWithSameCasing: Simulates an existing resource in state with user_name="user@example.com", then performs a Read where the API returns the same casing. Constructs a full InstanceState with computed set hashes and verifies that the Read produces exactly 2 access_control entries (no drift).

  • TestPermissionsDrift_UserNameNoDriftWithDifferentCasing: Simulates the exact drift from issue [ISSUE] Issue with databricks_permissions resource when user_name is used for access_control. There is a permanent drift with each terraform plan and apply #5183: InstanceState has user_name="user@example.com" but the API returns User@Example.com. Verifies that after the fix, the Read normalizes the casing so that:

    1. The access_control set still has exactly 2 entries (not 3, which would indicate drift)
    2. The user_name value in state is "user@example.com" (lowercase)

Both tests use MockWorkspaceClientFunc with mocked CurrentUser.Me() and Permissions.Get() responses, and construct realistic InstanceState maps with pre-computed set hashes to simulate the post-apply state that Terraform would produce.

  • make test run locally
  • relevant change in docs/ folder
  • covered with integration tests in internal/acceptance
  • using Go SDK
  • using TF Plugin Framework
  • has entry in NEXT_CHANGELOG.md file

Notes

  • No documentation changes needed. This is a bug fix with no new fields or behavioral changes to document.

jlieow and others added 2 commits May 28, 2026 09:43
@alexott alexott requested review from a team as code owners May 28, 2026 09:30
@alexott alexott requested review from simonfaltum and removed request for a team May 28, 2026 09:30
@alexott alexott temporarily deployed to test-trigger-is May 28, 2026 09:30 — with GitHub Actions Inactive
@alexott alexott temporarily deployed to test-trigger-is May 28, 2026 09:31 — with GitHub Actions Inactive
@alexott alexott requested review from chrisst and Copilot May 28, 2026 09:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes persistent drift in databricks_permissions when user_name / service_principal_name casing differs between Terraform config/state and the Databricks Permissions API response by treating these identity fields as case-insensitive.

Changes:

  • Normalize user_name / service_principal_name to lowercase in the access_control TypeSet hash function to prevent set element identity drift.
  • Normalize user_name / service_principal_name from API responses (Read) to lowercase, and switch current-user matching to case-insensitive comparisons.
  • Add unit tests covering same-casing vs different-casing API responses; add a changelog entry.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
permissions/resource_permissions.go Updates access_control set hashing to lowercase case-insensitive identity fields before hashing.
permissions/permission_definitions.go Uses case-insensitive comparison for current-user filtering and lowercases usernames/SPNs when flattening API response into state.
permissions/entity/permissions_entity.go Makes ContainsUserOrServicePrincipal case-insensitive via strings.EqualFold.
permissions/resource_permissions_test.go Adds regression tests reproducing and preventing casing-driven drift in Read/refresh behavior.
NEXT_CHANGELOG.md Adds bugfix entry for the drift issue.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +231 to 233
if strings.EqualFold(me, accessControl.UserName) || strings.EqualFold(me, accessControl.ServicePrincipalName) {
if !existing.ContainsUserOrServicePrincipal(me) {
continue
Comment thread NEXT_CHANGELOG.md Outdated
@alexott alexott temporarily deployed to test-trigger-is May 28, 2026 10:02 — with GitHub Actions Inactive
@alexott alexott temporarily deployed to test-trigger-is May 28, 2026 10:03 — with GitHub Actions Inactive
@alexott alexott temporarily deployed to test-trigger-is May 29, 2026 09:20 — with GitHub Actions Inactive
@alexott alexott temporarily deployed to test-trigger-is May 29, 2026 09:20 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown
Contributor

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/terraform

Inputs:

  • PR number: 5757
  • Commit SHA: 5c9a4485702576aaa009641f111e6abe4cb40a65

Checks will be approved automatically on success.

@alexott alexott added this pull request to the merge queue Jun 24, 2026
Merged via the queue into main with commit dcf2f73 Jun 24, 2026
12 checks passed
@alexott alexott deleted the fix/permissions-drift-user-name-alex branch June 24, 2026 12:21
deco-sdk-tagging Bot added a commit that referenced this pull request Jul 1, 2026
## Release v1.120.0

### New Features and Improvements
* Add resource and data source for `databricks_postgres_data_api`.

* Deprecate the SDKv2 fallback implementations of `databricks_library`, `databricks_quality_monitor`, and `databricks_share` resources, and the `databricks_share`, `databricks_shares`, and `databricks_volumes` data sources. These resources have been served by the Plugin Framework by default since their migration; the SDKv2 implementations remain only as opt-in fallbacks via the `USE_SDK_V2_RESOURCES` / `USE_SDK_V2_DATA_SOURCES` environment variables. Setting either environment variable for any of these names now emits a runtime warning (visible with `TF_LOG=WARN` or higher), and the SDKv2 implementations will be removed in the next major release of the provider.

### Bug Fixes

* Fix permanent permissions drift when `user_name` casing in `databricks_permissions` `access_control` blocks differs from the API response ([#5757](hattps://github.com//issues/5757)).
* Allow setting `user_api_scopes = []` on `databricks_app` to disable OBO (On-Behalf-Of) user authorization ([#5834](#5834)).

  The Apps API omits `user_api_scopes` from its response when OBO is inactive, so a configured empty list previously failed with `Provider produced inconsistent result after apply`. The provider now preserves a configured empty list in state, mirroring the reconciliation used by `databricks_app_space`.

### Internal Changes

* Make notification destination acceptance tests robust to the eventual consistency of the notification destinations list API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants